-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Server-side FFI bindings #3084
base: master
Are you sure you want to change the base?
Server-side FFI bindings #3084
Conversation
* Fix some compiler issues (signedness and pointer sizing) * Include What You Use * Sort the includes
Wow. Thanks for such an excellent write-up, it helps me understand what is being proposed far faster than trying to grasp 2000 LOC :) Add a server C API certainly is in scope (let me just link together some things, issue #2797 and previous PR #2807). I'll start with feedback on the write-up, and to preface all of that: I'm not an expert in writing C code. So, I may ask questions about something, but it's not to say that this is bad or wrong, but very likely is because I just don't have C API design experience, and thus welcome learning.
Is a channel-like thing the most idiomatic in C? As opposed to having the callback return a future (
Is this used always? Is it hard for a C user to provide some kind of
There is a new-ish PR here: hyperium/hyper-util#11
🙏 thank you, this is excellent! The previous thing was so hacky... I wonder if it'd be worth separating into it's own PR? Also, I now wonder if the single header file is wrong, since you can conditionally enable hyper's client or server. Should they be |
I did consider exposing a way for C code to create a generic OTOH the idea of using a "token" to corelate back to the context in which the work is happening is pretty common. As some examples
I actually didn't consider exposing each timer separately to the C code, though I've been discussing this option with some of the teams who want to switch to using hyper and they all felt much more comfortable using a timer heap written in rust than one written in C/C++ (along with maintaining the interface code between the two models). The timer heap I've added is based solely on stdlib code (no extra dependencies) and is designed to be as easy as possible to integrate with (it just tells the C code when to next call back into the executor so the C code only cares about one time, not all of them). Implementing the time heap in the Rust code allows the executor to check for timer-wakups while processing the runnable queue, which seems nice (and doesn't delay timers arbitrarily if the executor is busy) - see https://github.com/hyperium/hyper/pull/3084/files#diff-fc56f58b6249ca3abcf76fb3641d25425c53e4d9f6c56514fcfe81fef72c8d3aR147-R149
Ah, nice, shame that PR has to re-implement various hyper-internal things (like Rewind) whereas if this was part of hyper it can just use them directly. I'll also comment on that PR about the way connection configuration is done.
Can do - probably a good idea (though I'll not remove it from this MR yet, since people are already integrating with this branch internally 🙃)
Maybe, though features don't really integrate very well with This is basically the thinking which lead me to make the FFI option always enable |
Ok, cool, I started to suspect that when I wrote my question, but good to know you've looked at the existing patterns.
Exposing each timer separately? I'm not sure I follow. Maybe you were referring to what I'm about to describe, but: I meant exposing But it sounds like your users want to use a Rust timer anyways. I certainly get why (more in Rust is safer!). I'm currently undecided on the pros and cons of having it directly in hyper, versus having the heap timer be outside. It doesn't have to be decided immediately, we can choose to add it later, or choose to document that it might get moved out.
🙃 🥵
Why not? Isn't it typical to |
To be clear, the timer heap I've added is for Hyper's internal use (backing things like the "header timeout" function on HTTP/1 streams) it's not exposed to the C code to put general purpose timers on (I don't know if that matches your mental picture, but I thought I'd write it down explicitly just in case). When I said "exposing each timer separately" I was picturing something like the following - hyper might be processing multiple requests on multiple sockets each with multiple timeouts configured on them so the model where there's a In contrast, with the heap inside the rust code, the C code is simply told "hyper would like to be polled in
Fair enough, we can discuss this further down the line if needs be.
They're only doing de-risking prototyping work, but still, I'd prefer not to break their codebases if I can help it.
True, that is a common pattern, the difference as I see it here is that the What I'd want to avoid is there being a |
Ah yes, this makes sense. Let the exported symbols be consistent. We already have the start of this in the client code, there's a function enable HTTP/2, but if the feature was not compiled, it just returns an error, something like
Yea, I think I'm generally fine with all you've proposed. I'd just like to track its stabilization differently from the client FFI. Basically, I don't want us to have to decide the server stuff "is all good, freeze it forever" when stabilizing the client side for curl. |
Absolutely, I'm happy that all the changes I'm proposing here will fall into the "this API is unstable and is immune from Semver constraints" - the main goal of proposing this upstream is to get it out there so it hopefully can in time stabilize and, in the mean time won't be broken by non-FFI changes. |
Fix header handling for requests See merge request amc/hyper-ffi!2
Hi @seanmonstar! Hope all's going well. Investigations and implementation is continuing and they threw up an interesting problem that I wanted to flag with you. The basic problem is that when setting userdata on The original implementation of the C server code just magically "knew" that when a server connection task completed the IO userdata and service userdata that backed it were no longer needed and could be freed, and to achieve this those userdata pointers were duplicated in the task userdata. This design means that there are multiple references to the IO/service userdata alive at the same time and the code frees one set of them when it's "pretty sure" the other ones have been forgotten and won't be read again. This made our C developers rightly nervous 😨 so I've implemented a different strategy that's more similar to how Rust would manage the memory.
I'm raising this change specifically as it's the first breaking change to the existing client-facing bindings that this MR has introduced 😄 and hence I wanted to get your opinion on the approach. I will say that the related changes to the example server integration ( Additional Thoughts
|
Coming back to those additional thoughts, I've come to the conclusion that it's better to be consistent everywhere, so I've re-read your last comment (re: stability guarantees) and I may have misunderstood your question, are you proposing a I think the changes are now ready for review (the team that has been integrating with them has got their pre-existing test suite passing with these changes 😄 ). It would be great to try and get this in if possible (otherwise the branch will keep getting stale and needing catchup merges). |
Yes, that's probably the best way to do it. Whenever something does go stable, we just explicitly say which types/functions are stable in the changelog. |
Coming back to this, I've merged the latest master in to this branch and fixed up the discrepancies. Do you think you'll be able to review this soon to prevent it bit-rotting again? |
Continues the work started in #2278 by adding bindings to allow using hyper's server stack from a C runtime. The patterns are based on the model used on the client side, and the two can be used simultaneously (e.g. using the client bindings to make down-stream HTTP calls as part of serving an upstream request).
I understand that the project is currently focussed on the 1.0 stabilization (woo!) and I realise that this MR isn't part of that process, but there's various teams across my area of Microsoft who would really like to start using hyper in our C/C++ projects to take advantage of the benefits that hyper is already providing to our Rust codebases elsewhere. Thus it would be really great to get a first-pass review of the approach soon to try and head off any churn down the line, if possible?
I've added an example C integration (
capi/examples/server.c
) that simply logs information about the request then responds with a 404 to everything.Implementation details of note:
epoll_wait
's timeout argument though there is alsoepoll_pwait2
which goes down to nanoseconds so maybe the FFI should expose them?ffi
feature depend on all the other related features for now, mostly as a QoL for people using the FFI bindings (since the FFI code isn't feature aware yet)AutoConnection
layer that restores the behaviour of "trying HTTP/1.x and, if it sees the HTTP/2 prelude rewinds and switches to HTTP/2 mode". I've tried to make this generic so it is possibly in a fit state to be lifted to hyper-utils rather than being in the FFI code?Other smaller notes:
cargo expand
's ability to show a single module from a crate (so we no longer need to build a fake crate to be able to run bindgen)Sleep
auto-impl for things that look likeSleep
again as a QoL thingI've tested the server example code under
valgrind
and-fsanitize=memory
to ensure there's no use-after-free/double-free/etc.